-
-
Notifications
You must be signed in to change notification settings - Fork 46.6k
Documentation #8646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Documentation #8646
Conversation
for more information, see https://pre-commit.ci
…ed comments for better understanding
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
add documentation |
How is this PR different from #8633? |
In order to merge a PR, all test must pass, can you try to fix these errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does not add any information that the person reading the code cannot infer themselves - Seems pointless
if i not in temp: | ||
temp.append(i) | ||
## changed below lines added sets functionality and remove for loops to get unique elemnt in list | ||
temp = list(set(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a documentation change
"""This function takes two parameters: a string variable named | ||
'key' (with a default value of 'college') and a string variable | ||
named 'pt' (with a default value of 'UNIVERSITY'). | ||
The function returns a string that is a transformation of the 'pt' argument | ||
based on a key-shift cipher""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, this isn't inside of a function and isn't formatted
Secondly, what does this tell the user that we don't already know? Everything you said is clearly shown by typehints and naming conventions
print(r) | ||
print(len_temp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these prints
print(r) | |
print(len_temp) |
# for i in range(r*len_temp): | ||
# s = [temp[i] for _ in range(len_temp) if i < 26] | ||
# modalpha.append(s) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# for i in range(r*len_temp): | |
# s = [temp[i] for _ in range(len_temp) if i < 26] | |
# modalpha.append(s) |
Along with this random temp code
"""These lines of code creates a dictionary by iterating over the list of | ||
lists. Each letter in the alphabets list is mapped to a letter in a row of | ||
the "modalpha" list. The mappings are stored in the dictionary with the | ||
indices of the alphabets list as keys and the values fromthe corresponding modalpha lists as values""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be in the forms of comments, not docstrings
@CaedenPH this PR is basically identical to an earlier PR #8633 (same commits as this one), so one of the two should be closed. We can probably keep this PR since you already started a review on this one. There's also an even earlier PR #8626 by a different author that fixes the same issue #8622, so it'd be good if you could take a look at that one as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
Please make separate PRs for your changes to mixed_keyword_cypher.py
and your changes to the other files
Closed as duplicate of #8633 |
Describe your change:
Checklist:
Fixes: #{$ISSUE_NO}
.